feat(desktop): show Agent Graph output previews - #4751

Synthesis: GO, no P0–P2 (with 1 credibility note + 3 P3 observations)

This conclusion comes from @Ox-Qronos's independent review. I did not read this diff myself; I verified the current head has not drifted and the exact-head CI state.

What this diff actually does (16 files, three layers): per-operator bounded output previews (280 code points + 2KiB dual cap) with usage-derived TPS for Agent Graph projection; strict allowlist/decoder carriage of the optional output fields with compatibility epoch 133→134; Desktop panel rendering of live/result labels, TPS, and previews.

Credibility note on the PR body (not a code defect, but the author should know): the body claims the screenshot shows real output with measured TPS from a local build at ff866d713. Verification shows that commit only touches a coordinator test file (+35/-5) and contains no panel rendering (added 4 days later); the screenshot content matches storybook fixture data verbatim (session name, title, tokensPerSecond:42/18, mirrored story copy). So: real app window with synthetic data. Rendering itself matches the code path exactly, and feature validity is independently proven by local tests and hosted CI.

P3 observations:

1. Write amplification tradeoff: every text delta now materializes one bounded projection commit plus invalidation (the old design explicitly asserted zero projection writes on deltas). Payloads are bounded and semantics correct, but commit volume on high-frequency streaming sessions is worth knowing. The author already declared this tradeoff in the review focus.
2. Display semantic difference: durable rebuild truncates from the head while live streaming truncates from the tail, so the preview window flips from "start" to "end" on reconnect. Display layer only, authoritative data unaffected.
3. Dead code: the ?? textEvents[0]!.ts fallback near projection.ts:346 is unreachable (latestText is guaranteed non-empty). Harmless.

Not covered: real-provider streaming TPS behavior (no live streaming environment here); whether the author validated with real data beyond fixtures (only proven inconsistent with the claimed head, intent not proven); lint/format/knip not run locally (hosted CI green governs).

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

Re-verification on new head: GO stands, P3③ fixed

This follows up the review above. The PR has since moved to a new head with exactly one commit (fix(runtime): remove unreachable output timestamp fallback), which removes the dead-code fallback this review flagged as P3③. This conclusion comes from @Ox-Qronos's re-verification. I did not read this diff myself; I verified the head binding and the exact-head CI state below.

• The removal is behavior-preserving (the preceding guard guarantees a non-empty model text event, so the fallback was unreachable).
• Prior GO verdict (no P0–P2), the credibility note, and the remaining two P3 observations carry over unchanged.
• Exact-head test is green on this head.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

Re-verification on new head: GO stands

This follows up the reviews above. The PR has since moved to a new head containing two main merges plus three author-side changes. This conclusion comes from @Ox-Qronos's re-verification, rebuilt and re-run locally (5 suites green). I did not read this diff myself; I verified the head binding and the exact-head CI state below.

• Unified tail truncation (both durable and live paths now take the last 280 code points without trim) resolves the previously noted P3② display inconsistency, in the right direction and covered by tests.
• Panel visibility call updated for main's refactor; no behavior concern.
• Exact-head test is green on this head.

One action needed before merge: the declared compatibility epoch (137) is already taken on main (now at 141), so another rebase with a fresh epoch stamp will be required — the gate will enforce it, do not let it linger.

• Prior GO verdict (no P0–P2) and remaining observations carry over.

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

Review — head 3d7a4b1dd

Reviewed the effective diff against main (4410c3a2d) by reading the branch locally, line by line. I did not execute the test suites, so runtime-behaviour claims below are marked with confidence and should be confirmed by profiling before you act on the severity.

Scope read in full: stream-graph-projection.ts, stream-graph-read-model.ts, stream-graph-coordinator.ts, protocol/agent-graph.ts, protocol/index.ts, server/agent-graph-coordinator.ts, agent-graph-panel.tsx, agent-graph-copy.ts, agent-graph.css, the story, and all six touched test files.

The shape of this change is right: output previews are derived from existing RuntimeEvents, RuntimeEvents stay authoritative, the protocol boundary is allowlisted and byte-capped, and the epoch bump is correctly justified in the comment. The findings below are mostly about the presentation layer, which is where the test coverage thins out.

P2-1 — The ellipsis is rendered on the wrong side, so the UI states the opposite of the truth

The projection deliberately keeps the tail (packages/runtime/src/stream-graph-projection.ts, boundOutputPreview):

const visible = codePoints.slice(-AGENT_GRAPH_OUTPUT_PREVIEW_MAX_CODE_POINTS);
return { text: visible.join(''), truncated: true };

But the renderer appends … at the end (apps/desktop/src/renderer/agent-graph-panel.tsx):

{operator.output.preview}
{operator.output.previewTruncated ? '…' : ''}

Tail truncation means the beginning was dropped, so the ellipsis must lead (…{preview}). As written a long answer renders as "…the last 280 code points…", which reads as "more follows" when in fact the opening was cut. The existing instructionPreview in the same codebase is head-truncated with a trailing … (${work.instruction.slice(0, MAX_INSTRUCTION_PREVIEW_CHARS)}…) — that one is correct, and this change applies the same visual convention to the opposite truncation direction.

Three independent signals that this is a real defect and not my misreading:

1. agent-graph-panel.test.ts asserts assert.match(textContent, /Inspecting the renderer projection…/) while the fixture sets previewTruncated: true — the test freezes the incorrect rendering, so any fix must update that assertion too.
2. The OutputPreviews story hand-writes preview: 'Comparing the three provider adapters and their retry boundaries…' together with previewTruncated: false — the author was not settled on who owns the ellipsis.
3. Every truncation test (Array.from(text).slice(-280) in stream-graph-read-model.test.ts, the projection tests) asserts the projection string only. Nothing asserts the rendered ellipsis position.

P2-2 — "tokens/s" is whole-turn output ÷ whole-turn wall clock, and it disagrees with itself across paths

Batch/rebuild path (projectOperatorOutput):

const sampleStartedAt = orderedEvents.find(
  (event) => event.role === 'model' && event.content?.kind === 'text',
)!.ts;
const usageEndedAt = latestUsage?.ts;
const sampleDurationMs = Math.max(0, usageEndedAt - sampleStartedAt);

token_usage is emitted once per send, after the send finishes (ai-sdk-turn.ts), and its own comment states the value "spans every Runtime loop step and retry". So the numerator is the sum of output tokens across all steps of that send, while the denominator is first-token → end-of-turn — including every tool execution, thinking block, retry, and loop step. For a graph operator doing "Inspect the repository" work, the number will sit well below true generation speed, and it falls as the operator does more work. Rendered as 21.0 token/s it reads as generation throughput; it is not.

More concretely, the two paths disagree on the start of the sample window:

• incremental path: existing?.sampleStartedAt ?? event.ts (first delta this projection happened to observe)
• rebuild path: first text event in the stream

So the same operator's TPS changes across a projection repair (#repairClientProjectionBestEffort). Either unify the definition, or label it honestly (e.g. avg) — and if the metric is only meaningful for single-step runs, consider suppressing it otherwise. The PR description only acknowledges the per-delta write tradeoff, not this.

Minor related risk (medium confidence): the code takes usageEvents.at(-1), while the compaction path in runtime-kernel.ts emits a token_usage with output: 0. If that lands last in the stream, the outputTokens > 0 guard makes TPS disappear entirely rather than fall back to the previous sample.

P2-3 — Every text delta is now a full SQLite transaction plus an invalidation, with no bound on the backlog

Before this change text_delta returned undefined from projectClientSessionEvent, so advanceMaterializedAgentGraphClientProjection early-returned and wrote nothing. That is exactly the behaviour the deleted assertion protected:

'partial text deltas must not cause projection writes or invalidations'

Now every delta produces an output, so it runs the full commitAgentGraphClientProjection — a SQLite transaction that re-validates the version, writes an applied record, and rewrites the whole snapshot payload JSON and operator payload — and emits one runtime_activity invalidation.

#queueClientProjectionUpdate only chains onto the previous task; it neither coalesces nor drops:

const previous = driver.clientProjectionTask ?? Promise.resolve();
const task = previous.catch(() => {}).then(async () => { await operation(); });

At a few hundred deltas per second this serial queue grows without bound, each item costing O(snapshot). #waitForClientProjectionUpdates awaits the whole chain, so any repair path also waits out the entire backlog. The comment thread acknowledges "per-text-delta projection-write tradeoff remains explicit", but does not bound it. Since this is purely presentational data, dropping intermediate frames is safe — coalescing on a time window (say ≤100 ms) or keeping only the newest pending frame would remove the risk without changing semantics.

P3 observations

• activity silently became optional. AdvancedAgentGraphClientProjection.activity?: AgentGraphClientActivity is an exported type of packages/runtime. The single in-repo consumer (the coordinator) handles it, but this is a breaking type change for out-of-tree consumers and deserves a mention in the description/CHANGELOG.
• A re-run operator transiently loses its preview. projectOperatorOutputs keeps only the stream with the newest openedAt, and projectOperatorOutput returns undefined until that activation emits text — so a previously visible "Result preview" disappears rather than falling back to the last settled activation. Worth deciding deliberately.
• latestEventTime semantics widened. In the output-only branch, snapshot.latestEventTime = Math.max(..., runtime.event.ts) lets a partial delta's ts advance a field that previously only reflected committed non-partial records. I grepped apps/desktop/src and found no consumer, so it is currently harmless, but the field no longer means "last committed fact".

What is done well

• The startOffset guard in foldClientOutputDelta is correct and non-obvious: startOffset is a UTF-16 offset (events.ts), and once the preview has been cut to a tail, offset arithmetic is meaningless. The author correctly avoids mixing the two.
• The ! assertion on sampleStartedAt is genuinely safe — latestText comes from the non-partial subset and orderedEvents is a superset, so find must hit. I verified 44366b7d4: the "unreachable fallback" the comment thread says was removed really was removed, and the assertion really is sound.
• Protocol boundary is solid: allowlisted requireShapedRecord, requireNonNegativeFiniteNumber rejecting negative TPS with a matching assertion, and the privateOutput leak test in agent-graph-coordinator.test.ts.
• The 280-code-point producer cap versus the 2 KiB consumer byte cap is a real double guard, not duplication — 280 four-byte emoji is 1120 B, comfortably under 2048 B.

Verdict

Direction and layering are right, and the protocol work is careful. I would not merge before P2-1 is fixed — it is user-visible misinformation, and the current test freezes it. P2-2 needs at minimum an honest label or a unified definition across both paths. P2-3 needs a bound on the queue. P3 items are the author's call.

The authority story is clean: previews are derived from existing immutable RuntimeEvents with no new durable facts, the 280-code-point bound counts code points correctly (Array.from, surrogate-pair safe), the token rate is honestly labeled as a window average including tool time, the strict decoder whitelists keys and rejects unknown shapes, and the epoch bump is necessary since old strict decoders would reject the new output field outright. The 100ms coalescing window preserves ordering (same runId+messageId+contiguous offset only) and terminal-state correctness.

One P1 inline, one P2 and P3s in body.

P2 — the coalescing regression is not actually pinned (normal path). The test asserts outputProjectionCommits < 60, but the FakeBackend emits ~18 deltas in that scenario (9 chars / 45ms apart), so removing the 100ms window still passes. Either inject >60 deltas or tighten the assertion to the delta count.

P3s:

• A stale-activation late delta can overwrite a new run's output (no event.ts < previewUpdatedAt guard when existing mismatches activationId) — transient, self-heals on rebuild. ②
• Incremental vs replay derivation diverge slightly: token_usage arriving before any text is dropped by advanceClientOperatorOutput but kept by projectOperatorOutput; sampleStartedAt differs when the first event is text_complete; pre-activation output retention differs. Same state renders differently across paths; converges eventually.
• The 280 bound is defined twice (AGENT_GRAPH_OUTPUT_PREVIEW_MAX_CODE_POINTS vs MAX_OUTPUT_PREVIEW_CODE_POINTS) — one constant should own it.
• isMaterializedGraphClientEvent dropping 'text_delta' is unreachable (the only call site is gated earlier); if the predicate is reused later it bypasses coalescing.
• A non-contiguous startOffset gap replaces the preview without marking it truncated — shows a fragment with no ellipsis.
• projectOperatorOutputs sorts all events per run on the observe hot path.
• Renderer nits from the panel side: preview lacks <bdi> isolation (a precedent exists in TailVisibleLabel); the literal … is not aria-hidden; a failed operator can keep showing "Live output" until the next durable rebuild; unrelated model-picker-internals.tsx whitespace churn should be split or dropped.

Reviewed current head f7b1dbe. Agent Graph output previews and average throughput are bounded, protocol-guarded, and covered by the focused Runtime/Host checks. No P0–P2 issue found.